Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update series.py #485

Merged
merged 5 commits into from
Jan 3, 2024
Merged

Update series.py #485

merged 5 commits into from
Jan 3, 2024

Conversation

neillinehan
Copy link
Contributor

Fixes #484 (comment)

ntau not in wwz_coherence() arguments, so pop it out of the settings dict after calculations are complete.

Fixes part-2 of issue: LinkedEarth#484

Move ntau-pop up to avoid errors where ntau does not exist.

Add condition for the scenario where 'tau' is in settings, (as is the case when signif_test() calls wavelet_coherence()).
Fixed minor string issue.
@CommonClimate
Copy link
Collaborator

Hi @neillinehan thanks for suggesting this fix. Did you end up creating a pytest for it as well? This is something we encourage for any change to the code, and you already have all it takes to make one. Another thing is that you need to nominate someone to review your code. I think @fzhu2e would be best if he has time, as this functionality is his baby, and he knows that part of the code the best.
Out of curiosity, what are you trying to do with the code?

@neillinehan
Copy link
Contributor Author

Hi @CommonClimate,

The proposed fix works in my local Jupyter notebook environment. I’ve tested the changes informally and everything works as expected. I'm new to GitHub and finding the process of writing formal tests a bit time-consuming at the moment, I understand if this means my current contribution cannot be accepted. I may return to this at a later time.

I'm exploring seasonal changes in some crop light responses sampled at a fairly high frequency (1-min), so I need higher time-resolution as opposed to the default high spectral-resolution.

@CommonClimate
Copy link
Collaborator

Hi @neillinehan ,
I understand unit tests may not be your bread and butter, but as it stands I have no easy way to test the veracity of your claim ("The proposed fix works in my local Jupyter notebook environment"), or more importantly that it works in an arbitrary environment. Would you be able to repurpose the docstring example using AIR and NINO3 and making sure that your fix works for that? Please share your code, if so, and I'll turn that into a unit test prior to merging.
Thank you in advance for easing our process!
Best,
Julien

@CommonClimate CommonClimate self-assigned this Jan 3, 2024
@CommonClimate CommonClimate self-requested a review January 3, 2024 04:19
Copy link
Collaborator

@CommonClimate CommonClimate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after adding 2 unit tests to ensure that the changes a) don't break anything and b) work as advertised.

@CommonClimate
Copy link
Collaborator

addresses issue #484

@CommonClimate CommonClimate merged commit 645f839 into LinkedEarth:master Jan 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

passing ntau to wavelet_coherence causes an error
2 participants